feat: deprecation warnings on select application attributes #1116
Conversation
dimaqq
force-pushed
the
feat-application-deprecation
branch
from
26be5d0
to
369eb96
Compare
dimaqq
force-pushed
the
feat-application-deprecation
branch
2 times, most recently
from
e4b9a8b
to
1ec66d5
Compare
|
This package already imports typing_extensions via some dependency. I've made it an explicit dep, allowing for a simpler Here's a test run, showing correct source for the warning: /code/python-libjuju/example.py:28: DeprecationWarning: Application.owner_tag is deprecated and will be removed in v4
owner_tag.......... {app.owner_tag!r}
/code/python-libjuju/example.py:30: DeprecationWarning: Application.min_units is deprecated and will be removed in v4
min_units.......... {app.min_units!r}
/code/python-libjuju/example.py:32: DeprecationWarning: Application.subordinate is deprecated and will be removed in v4
subordinate........ {app.subordinate!r}
/code/python-libjuju/example.py:34: DeprecationWarning: Application.workload_version is deprecated and will be removed in v4, use Unit.workload_version instead.
workload_version... {app.workload_version!r} |
|
I see doc changes as orthogonal, so maybe in a separate PR? |
|
CI failures are unrelated, #1108 |
There was a problem hiding this comment.
The @deprecated decorator is cool, didn't know about that. One or two result in rather long lines, but I think we can look at that later if we introduce linting with ruff/etc.
The CI failures are all known failing tests. 3/5 fail 100% of the time on main.
test_wait_for_idle_more_units_than_needed fails 80% of the time on main.
test_subordinate_false_field_exists fails around 25% of the time on main.
The changes made in this PR seem reasonable to me, but I would like the source of truth for the new property names and their type annotations to be mentioned somewhere, if not in a docstring then perhaps somewhere it will show up in the git log.
"""Class representing a juju application.
Explicit properties are annotated with data types inferred from ...
See ... juju documentation page?
"""
I think I want this because I don't know where they come from or why they're correct, and maybe a future reader will wonder the same thing. But if you don't think this is worthwhile here then let's skip it for now.
Also, maybe we want a <5 restriction on the typing_extensions dependency just to be conservative? I know the existing dependencies are loosely specified, but that already caused at least one CI breakage.
dimaqq
force-pushed
the
feat-application-deprecation
branch
from
1ec66d5
to
b359e0d
Compare
Done, ptal |
|
/build |
|
/merge |
dimaqq
force-pushed
the
feat-application-deprecation
branch
from
186ab56
to
5de27a9
Compare
|
/merge |
Description